cache: sentinel to invalidate the chunk index during fragment deletion#9908
cache: sentinel to invalidate the chunk index during fragment deletion#9908mr-raj12 wants to merge 4 commits into
Conversation
…ex fragments (borgbackup#9904) An interrupted fragment deletion could leave a subset that build_chunkindex_from_repo trusts as a complete index; the sentinel now forces a rebuild from packs instead.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9908 +/- ##
==========================================
+ Coverage 85.13% 85.28% +0.15%
==========================================
Files 93 93
Lines 15899 15932 +33
Branches 2428 2435 +7
==========================================
+ Hits 13535 13588 +53
+ Misses 1654 1635 -19
+ Partials 710 709 -1 ☔ View full report in Codecov by Harness. |
ThomasWaldmann
left a comment
There was a problem hiding this comment.
in a future PR, we will also need to refactor these functions into a class dealing with index management.
…pace Store the marker as config/chunkindex-invalid instead of an all-zeros index/ fragment name, so it can never collide with a real fragment; detect it via a config listing.
ThomasWaldmann
left a comment
There was a problem hiding this comment.
Reviewed the current state of the branch (through d344e6e). Overall this is in good shape: the out-of-band config/chunkindex-invalid marker is a clean design, the marker-before-delete ordering is correct, and I chased the concurrent interleavings (two shared-lock clients rolling forward; a roll-forward racing another client's close-write) without finding a way to end up with a wrong index — every window leaves either the marker in place or zero fragments, both safe states. The slow rebuild inserting everything with F_NEW also means a roll-forward client persists the full rebuilt index at close(), so the repo is never left with only a tiny incremental fragment being trusted as complete. Applied the diff locally and ran cache_test.py, repository_test.py, compact_cmd_test.py, check_cmd_test.py: 156 passed.
Two things I'd like settled before merge, one follow-up, and two nits:
1. Plain borg check and borg compact --dry-run now write to the repo. The roll-forward in build_chunkindex_from_repo() runs on any index build, so with a leftover marker, borg check (via ArchiveChecker) and borg compact --dry-run delete fragments plus marker under a shared lock — commands that promise not to modify anything. (The repo-check warning even says "will be rebuilt on next use", and then check's own archive phase is that next use and mutates the repo.) Suggest a rollforward=True kwarg on build_chunkindex_from_repo(), passed as False by the checker and by dry-run compact — those callers would still break to the slow rebuild, just without deleting. Alternatively, if finishing an interrupted deletion is considered acceptable maintenance even from these commands, a comment saying so explicitly would do.
2. Version-skew trade-off of the out-of-band marker. An earlier revision's in-band index/000…0 sentinel made any older borg2 client fail the fragment merge (unreadable "fragment") and fall back to the safe slow rebuild. With the marker in config/, an older client on a shared repo sees only the leftover fragments and trusts them as a complete index — the exact #9904 failure, now reachable through version skew. Probably acceptable at this stage, but it should be a deliberate choice, and a changelog note recommending clients upgrade together would cover it.
3. (follow-up material) Guard window in write_chunkindex_to_repo. The marker is written after the new fragments are stored, so a crash between storing and the marker write leaves old + new fragment sets coexisting unguarded; the merge is last-write-wins in fragment-name order, so deleted entries can resurrect nondeterministically. Today this is only reachable via Repository.delete(update_index=True) (i.e. borg debug delete-obj), since compact wipes the index up-front — but moving the guard write before the store loop would close it for free, or at least a comment acknowledging the window.
Nits:
- No test interrupts the guarded
delete_otherdeletion insidewrite_chunkindex_to_repoitself; interruption coverage is only ondelete_chunkindex_from_repo. - The happy path now costs one extra
store_list("config")round-trip per index build (and one per repack, i.e. per backup close). Negligible locally, measurable on high-latency remotes; fine to leave as is.
Compatibility details I checked and that are fine: config/ already holds variable-name objects (space-reserve.*) and every consumer filters by name/prefix, so the marker can't confuse anything; repository.check() doesn't content-verify config/ objects, so the empty marker can't be flagged as corruption; the function-level from .cache import chunkindex_is_invalid in repository.py matches the existing pattern there (top-level would be circular).
🤖 Review drafted with Claude Code
|
3 and borg debug delete-obj, leaving as discussed, also moved the marker to the cache/ namespace in ea1d86d. compact filters its cache/ listing by the referenced-by-archive prefix and the only other object there is last-pack-checked, so there is no name clash. |
What
Deleting chunk index fragments is not crash safe: if the delete is interrupted partway, the leftover fragments look like a complete index and borg trusts them, so the index ends up missing entries or keeping stale ones.
This writes an "index invalid" marker before the first fragment delete and removes it after the last. While the marker is present, the index is rebuilt from packs on next load.
The marker lives out of band as a
cache/chunkindex-invalidobject, not in theindex/namespace, so it can never collide with a realindex/<sha256>fragment. Its presence is detected with acache/listing rather than a load, and listing bypasses the store cache, so this stays correct even ifcache/gets a cache configuration later.The marker only guards non-superset rewrites (full replacements and the invalidate-all path). Repack does not need it, since the fragments it writes already contain the entries it deletes.
Closes #9904